Fix hdrHeap/hdrStrHeap allocator inuse metric underflow#13218
Conversation
The hdrHeap and hdrStrHeap allocators are plain Allocator globals whose
objects are allocated with no constructor arguments, so THREAD_ALLOC
resolves to the non-templated thread_alloc(Allocator &, ProxyAllocator &)
overload. Unlike the templated overload, it did not call
increment_for_alloc() when reusing an object from the per-thread
freelist, while THREAD_FREE always decremented inuse on the way in.
The counted frees therefore outran the counted allocs and
proxy.process.allocator.inuse.{hdrHeap,hdrStrHeap} marched negative,
wrapping around as a huge uint64 value. Other allocators use
ClassAllocator (the templated path) and were unaffected.
Account for the freelist reuse in the non-templated overload so the two
paths stay symmetric, and add a regression test that cycles a block
through the freelist and asserts inuse stays balanced and never dips
below its starting value.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes allocator inuse metric underflow for hdrHeap / hdrStrHeap when TS_USE_ALLOCATOR_METRICS is enabled by making the non-templated thread_alloc(Allocator &, ProxyAllocator &) update metrics consistently with the templated overload, and adds a regression unit test to prevent recurrence.
Changes:
- Update the non-templated
thread_alloc(Allocator &, ProxyAllocator &)freelist-reuse path to callincrement_for_alloc()underTS_USE_ALLOCATOR_METRICS. - Add a Catch2 regression test that repeatedly frees/reuses a single block via the per-thread freelist and asserts
inuseremains balanced forhdrHeapandhdrStrHeap. - Add metrics-related includes and helpers in the HdrHeap unit test (guarded by
TS_USE_ALLOCATOR_METRICS).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/iocore/eventsystem/ProxyAllocator.cc |
Fixes allocator metrics accounting symmetry on freelist reuse for the non-templated thread_alloc. |
src/proxy/hdrs/unit_tests/test_HdrHeap.cc |
Adds a regression test to validate inuse remains stable across freelist free→reuse cycles for hdr heap allocators. |
A failing REQUIRE()/CHECK() throws and would skip the manual restore of the cmd_disable_pfreelist harness flag, contaminating later tests in the same process. Use an RAII guard that preserves and restores the original int value on scope exit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Approve. Correctly fixes the inuse underflow: the non-templated thread_alloc freelist-reuse path was missing the increment_for_alloc() that THREAD_FREE always pairs with, so frees outran allocs and inuse.{hdrHeap,hdrStrHeap} wrapped. Restores symmetry with the templated overload; regression test (RAII-guarded cmd_disable_pfreelist) is solid. CI green.
bryancall
dismissed
their stale review
Duplicate review double-posted by tooling — dismissing; the identical approval below stands.
Problem
With
ENABLE_ALLOCATOR_METRICS=ON,proxy.process.allocator.inuse.hdrHeapand
proxy.process.allocator.inuse.hdrStrHeapgo deeply negative —observed wrapping around as a huge
uint64value (displayed ~ -5.8e10).Every other allocator's
inusestays sane.Root cause
hdrHeapAllocatorandstrHeapAllocatorare plainAllocatorglobals,and their objects are allocated with no constructor arguments
(
THREAD_ALLOC(hdrHeapAllocator, this_ethread()), then a manualinit()/placement-new). With no extra args,THREAD_ALLOCresolves tothe non-templated overload
thread_alloc(Allocator &, ProxyAllocator &)inProxyAllocator.ccrather than the templated
thread_alloc(CAlloc &, ProxyAllocator &, Args &&...)in
ProxyAllocator.h.The two overloads diverged:
ClassAllocator<>allocator —IO buffers, sessions, etc.) calls
a.increment_for_alloc()when itreuses an object from the per-thread freelist.
Meanwhile
THREAD_FREEalways decrementsinuse(UPDATE_FREE_METRICS)when an object is pushed onto the freelist. So for these two allocators,
every reuse-from-freelist decremented
inusewith no matching increment,and the counter marched negative until it wrapped. Only
hdrHeapandhdrStrHeaphit the non-templated overload, which is why only theyunderflowed.
Fix
Add the missing
increment_for_alloc()to the freelist-reuse path of thenon-templated
thread_alloc, mirroring the templated overload, so the twopaths stay symmetric. Guarded by
TS_USE_ALLOCATOR_METRICS.Test
Added a regression test in
test_HdrHeap.cc(guarded byTS_USE_ALLOCATOR_METRICS) that enables the per-thread freelist and cyclesa single
hdrHeap/hdrStrHeapblock through free→reuse 1000 times,asserting
inusereturns to baseline+1 and never dips below the startingvalue.
Before the fix the test fails as
inusereaches-999/-1000(reproducing the underflow); after the fix it passes. The full
test_proxy_hdrssuite is green.🤖 Generated with Claude Code